Button: Creating ActionButton variant and cleaning styling #8645

khmakoto · 2019-04-08T21:54:36Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ npm run change

Description of changes

This PR adds an ActionButton variant to improve parity between new and old Button components while also making some styling fixes in general.

Below is a comparison, on the left using the ActionButtons in office-ui-fabric-react and on the right using the ones in experiments:

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

size-auditor · 2019-04-08T22:23:14Z

Size Auditor did not detect a change in bundle size for any component!

…c-react into actionButton

JasonGore · 2019-04-09T17:00:06Z

packages/experiments/src/components/Button/ButtonVariants.types.ts


 export interface IButtonVariantProps extends IButtonProps {
+  iconProps?: IIconProps;


I think we should consider marking this prop as deprecated in master and then remove it for Fabric 7 release, which would mean we wouldn't need to add it here.

But how can we mark it as deprecated in the current Button in oufr if we still don't offer an alternative in that production component?

By adding a content prop.. should be relatively easy. Unless we don't think that's a good permanent name for Button content, in which case we have a larger problem to address.

And then we do the same for iconProps? Deprecate it in favor of a new icon prop?

Possibly. It's either that or we carry over duplicate names for props even in Fabric 7 which seems cumbersome to take on for all of 7.x. I wonder what @dzearing thinks.

JasonGore · 2019-04-09T17:13:53Z

packages/experiments/src/components/Button/PrimaryButton.tsx


-  return <Button primary content={text} {...rest} />;
+  return <Button primary content={text} icon={iconProps} {...rest} />;


What is the name that appears in the hierarchy for these variants? If it's Button then styling and theming targeting PrimaryButton by name won't work. If both Button and PrimaryButton are appearing in hierarchy, then I think styling and theming will be even more wonky.

All of this is just driving need for an extendComponent utility that will just have one entry called PrimaryButton in the hierarchy that should still take directed styling and theming. This is covered by #8331 (and it seems should be resolved before Fabric 7 release.)

Yes, I think only Button appears in the hierarchy right now.

The other button variants appear as Unknown. We could apply displayName to them but that won't fix the styling issue since there is no helper like styled reading the setting and applying it (the variants are just thin wrappers.)

I'll add a note to #8331. The only other solution I can think of is to create new variants using styled or createComponent.

JasonGore · 2019-04-09T17:14:41Z

packages/experiments/src/components/Button/DefaultButton.tsx

@@ -3,7 +3,7 @@ import { Button } from './Button';
 import { ButtonVariantsType } from './ButtonVariants.types';

 export const DefaultButton: ButtonVariantsType = props => {
-  const { text, ...rest } = props;
+  const { text, iconProps, ...rest } = props;


I didn't catch this in the other PR but maybe text is another prop that could be deprecated in master and removed in Fabric 7.

msft-github-bot · 2019-04-09T17:24:22Z

Hello @kkjeer!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me and give me an instruction to get started! Learn more here.

kkjeer · 2019-04-09T17:24:41Z

@msft-github-bot require approval from @JasonGore and @dzearing

msft-github-bot · 2019-04-09T17:24:44Z

Hello @kkjeer!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @JasonGore and @dzearing

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

khmakoto · 2019-04-09T20:41:02Z

@msft-github-bot forget everything

msft-github-bot · 2019-04-09T20:41:03Z

Hello @khmakoto!

Because you've told me to reset the custom auto-merge settings, I'll use the configured settings for this repository when I'm merging this pull request.

khmakoto · 2019-04-09T20:41:14Z

@msft-github-bot require approval from @JasonGore

msft-github-bot · 2019-04-09T20:41:15Z

Hello @khmakoto!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @JasonGore

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

JasonGore

Approving noting that styling and theming won't work until #8331 is resolved.

msft-github-bot · 2019-04-10T12:35:17Z

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

khmakoto added 2 commits April 8, 2019 14:49

Button: Creating ActionButton variant and cleaning styling.

0110e72

Adding change file.

19a0e17

khmakoto added Component: Button Package: experiments labels Apr 8, 2019

khmakoto requested review from dzearing and JasonGore April 8, 2019 21:54

msft-github-bot assigned kkjeer Apr 8, 2019

khmakoto added 2 commits April 8, 2019 16:20

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

cca2e83

…c-react into actionButton

Fixing padding.

b6e57de

JasonGore reviewed Apr 9, 2019

View reviewed changes

kkjeer added the AutoMerge label Apr 9, 2019

micahgodbolt mentioned this pull request Apr 9, 2019

Refactor: Button #6134

Closed

37 tasks

JasonGore mentioned this pull request Apr 10, 2019

Foundation: Extend Component #8331

Closed

JasonGore approved these changes Apr 10, 2019

View reviewed changes

msft-github-bot merged commit 9834754 into microsoft:master Apr 10, 2019

khmakoto deleted the actionButton branch April 10, 2019 00:13

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Button: Creating ActionButton variant and cleaning styling #8645

Button: Creating ActionButton variant and cleaning styling #8645

khmakoto commented Apr 8, 2019 •

edited

Loading

size-auditor bot commented Apr 8, 2019

JasonGore Apr 9, 2019

khmakoto Apr 9, 2019

JasonGore Apr 9, 2019

khmakoto Apr 9, 2019

JasonGore Apr 9, 2019

JasonGore Apr 9, 2019

khmakoto Apr 9, 2019

JasonGore Apr 10, 2019 •

edited

Loading

JasonGore Apr 9, 2019

msft-github-bot commented Apr 9, 2019

kkjeer commented Apr 9, 2019

msft-github-bot commented Apr 9, 2019

khmakoto commented Apr 9, 2019

msft-github-bot commented Apr 9, 2019

khmakoto commented Apr 9, 2019

msft-github-bot commented Apr 9, 2019

JasonGore left a comment

msft-github-bot commented Apr 10, 2019


		export interface IButtonVariantProps extends IButtonProps {
		iconProps?: IIconProps;


		return <Button primary content={text} {...rest} />;
		return <Button primary content={text} icon={iconProps} {...rest} />;

Button: Creating ActionButton variant and cleaning styling #8645

Button: Creating ActionButton variant and cleaning styling #8645

Conversation

khmakoto commented Apr 8, 2019 • edited Loading

Pull request checklist

Description of changes

Focus areas to test

Microsoft Reviewers: Open in CodeFlow

size-auditor bot commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonGore Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-github-bot commented Apr 9, 2019

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me and give me an instruction to get started! Learn more here.

kkjeer commented Apr 9, 2019

msft-github-bot commented Apr 9, 2019

khmakoto commented Apr 9, 2019

msft-github-bot commented Apr 9, 2019

khmakoto commented Apr 9, 2019

msft-github-bot commented Apr 9, 2019

JasonGore left a comment

Choose a reason for hiding this comment

msft-github-bot commented Apr 10, 2019

khmakoto commented Apr 8, 2019 •

edited

Loading

JasonGore Apr 10, 2019 •

edited

Loading